Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the “List Letters” flow in the API handler to source pending letters from the new letter queue (DynamoDB) rather than querying the main letters table, and introduces queue “visibility” updates to temporarily hide claimed letters.
Changes:
- Replace supplier letter listing with
getPendingLetters()backed byLetterQueueRepositoryand a visibility timeout mechanism. - Add new datastore types and repository methods to query the queue and update visibility timestamps.
- Wire new env vars, dependencies, tests, and Terraform IAM permissions/env configuration for the queue table.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| lambdas/api-handler/src/services/letter-operations.ts | Adds getPendingLetters() and maps queue items to LetterBase while updating visibility. |
| lambdas/api-handler/src/services/tests/letter-operations.test.ts | Updates service tests for the new pending-letters API. |
| lambdas/api-handler/src/handlers/get-letters.ts | Switches handler to use getPendingLetters() and queue visibility timeout. |
| lambdas/api-handler/src/handlers/tests/get-letters.test.ts | Updates handler tests to expect getPendingLetters() usage. |
| lambdas/api-handler/src/config/env.ts | Adds required queue env vars (table name, TTL, visibility timeout). |
| lambdas/api-handler/src/config/deps.ts | Adds letterQueueRepo to dependency container. |
| lambdas/api-handler/src/config/tests/env.test.ts | Updates env parsing tests for new required vars. |
| lambdas/api-handler/src/config/tests/deps.test.ts | Asserts LetterQueueRepository is constructed with correct config. |
| internal/datastore/src/types.ts | Introduces PendingLetterBase and redefines InsertPendingLetter. |
| internal/datastore/src/letter-repository.ts | Removes legacy supplier/status list querying methods. |
| internal/datastore/src/letter-queue-repository.ts | Adds getLetters() and updateVisibilityTimestamp() for queue operations. |
| internal/datastore/src/test/letter-repository.test.ts | Removes tests for removed letter-listing methods. |
| internal/datastore/src/test/letter-queue-repository.test.ts | Adds tests for queue querying and visibility updates. |
| infrastructure/terraform/components/api/module_lambda_get_letters.tf | Updates IAM to query/update the queue table/index instead of letters table/index. |
| infrastructure/terraform/components/api/locals.tf | Adds queue-related env vars to Lambda configuration. |
francisco-videira-nhs
left a comment
francisco-videira-nhs
left a comment
There was a problem hiding this comment.
LGTM - just a comment about globals.
I was wondering if suppliers using the current endpoint would find any difference when we switch to using the letter queue, but since the letter queue is like a mirror of pending letters, I don't think they would so there doesn't seem to be any risk there?
| }); | ||
| }); | ||
|
|
||
| describe("getLetters", () => { |
There was a problem hiding this comment.
Think it's an older issue, but on my end the editor can't find some of these jest globals.
We can either import them
import { describe, test, expect, jest } from "@jest/globals"; in each file
or
add them to tsconfig.json
"compilerOptions": {
"types": [
"jest"
]
},
There was a problem hiding this comment.
Hmm, interesting, it's fine in my editor. I find closing and re-opening vscode fixes most issues like this. Unless I did else something to fix this at some point and I've just forgotten?
Adding the import to each file would make the issue go away, I'd expect, but it shouldn't be necessary. I don't pretend to understand how tsconfig.json works, but I think your suggestion would restrict the use of globals to jest, that is prevent them from being used from other packages.
No, we just need to ensure enough time has passed that all the letters in the letter table were inserted after we started populating the letter queue. (Which we're doing by putting them in different releases) |
Description
Context
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.